Skip to content

Close stdio input before waiting for server exit - #1838

Open
luisangelrod wants to merge 2 commits into
modelcontextprotocol:mainfrom
luisangelrod:fix/1836-graceful-stdio-shutdown
Open

Close stdio input before waiting for server exit#1838
luisangelrod wants to merge 2 commits into
modelcontextprotocol:mainfrom
luisangelrod:fix/1836-graceful-stdio-shutdown

Conversation

@luisangelrod

@luisangelrod luisangelrod commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • close the child process's standard input before waiting for stdio server shutdown
  • allow well-behaved servers that exit on EOF to complete graceful cleanup without consuming ShutdownTimeout
  • add a regression test that verifies disposal observes a clean server exit after stdin EOF without relying on wall-clock timing

Validation

  • dotnet test tests/ModelContextProtocol.Tests/ModelContextProtocol.Tests.csproj --filter FullyQualifiedName~StdioClientTransportTests — 54 passed on each of net472, net8.0, net9.0, and net10.0 (216 total)
  • dotnet build -c Release — succeeded with 0 warnings and 0 errors
  • the regression scenario was verified to fail against the previous implementation after ShutdownTimeout and pass with this change by observing a clean exit code of 0, without asserting wall-clock duration

The full test suite was also attempted locally. Unrelated environment dependencies involving the missing Node conformance helper and a localhost OAuth development certificate prevented a clean full-suite completion. One parallel task-cancellation test also failed during that run and passed when rerun in isolation.

Fixes #1836

Note

This pull request description and implementation were prepared with AI assistance.

@luisangelrod
luisangelrod force-pushed the fix/1836-graceful-stdio-shutdown branch from b01139d to 620d49c Compare August 31, 2026 11:51
@luisangelrod

Copy link
Copy Markdown
Contributor Author

The current Windows Debug failure is unrelated to the stdio transport change. The two failed tests were:

  • OAuth.TokenCacheTests.GetTokenAsync_ExplicitClientDoesNotRefreshTokenIssuedToDifferentClient, which could not discover localhost OAuth metadata
  • AuthorizeAttributeTests.ListResourceTemplates_WithHandlerAndNullCollection_AllResourceTemplatesVisible, which received mismatched protocol versions

The changed StdioClientTransportTests had no failures. Windows Release and both configurations on Ubuntu and macOS passed, and the focused stdio suite passes 216/216 locally. GitHub did not permit me to rerun the failed job from the fork, so a maintainer rerun may be needed.

@luisangelrod
luisangelrod marked this pull request as ready for review August 31, 2026 12:24
@halter73
halter73 self-requested a review September 4, 2026 00:12

@halter73 halter73 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking this down, filing such a thorough issue, and following through with a focused fix and regression test. I left one small comment about logging the failed close path, but otherwise this looks great.

{
_process.StandardInput.Close();
}
catch { }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we log in the unlikely event this catches anything? I was tempted to suggest removing the catch since closing stdin should succeed even after the process exits, but preserving the fallback kill seems safer. A warning like "{EndpointName} failed to close server standard input; continuing shutdown." would work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stdio client never closes the server's stdin, so every client dispose burns the full ShutdownTimeout (5s by default)

2 participants